Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include already-installed matching requirements in the to_install set, so that pip-sync reliably installs dependencies #907

Closed
wants to merge 1 commit into from
Closed

Include already-installed matching requirements in the to_install set, so that pip-sync reliably installs dependencies #907

wants to merge 1 commit into from

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Sep 23, 2019 •
edited
Loading

Have diff include already-installed matching requirements in its to_install set.

This means that during sync, dependencies of desired packages will be (re)installed after potentially being uninstalled, regardless of environment state at time of sync, for a more consistent, predictable, and practical resulting state. Fixes #896.

If this is not desired, --no-deps can be provided via the new --pip-args option, whereby it will be passed through to pip install, and un-locked deps will be either removed or not installed, to match the lockfile exactly.

The following existing tests check the to_install set, and are herein adjusted to expect the inclusion of explicit requirements (all in test_sync.py):

  • test_diff_should_do_nothing
  • test_diff_should_uninstall
  • test_diff_should_uninstall_with_markers
  • test_diff_leave_packaging_packages_alone
  • test_diff_leave_piptools_alone
  • test_diff_with_matching_url_versions

Note: test_diff_with_matching_url_versions, before this change, expects pip-tools to prevent reinstallation of versioned URL requirements, but this is unnecessary as pip itself will do that, reporting "Requirement already satisfied . . .", and the test's comment is updated to reflect that. If this is incorrect under some circumstance, please correct me!

Changelog-friendly one-liner: pip-sync now ensures installation of dependencies of a locked requirement, even if those deps are not locked and the locked requirement is already installed; this can be disabled with pip-sync --pip-args=--no-deps.

Contributor checklist
  • Provided the tests for the changes. (Well, modified existing tests)
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

If this were to be accepted, would the appropriate merge point be the next major version, as the behavior may not be what is currently expected? I view it as a bug fix, and do not know when or why the current behavior would be desired instead, but I am only me.

@atugushev Please have a look for discussion and review. Thanks!

@atugushev atugushev changed the title Bugfix/896 - Have diff include already-installed matching requirements in its to_install set. Have diff include already-installed matching requirements in its to_install set. Sep 26, 2019
@atugushev atugushev added the needs discussion Need some more discussion label Sep 26, 2019
@codecov

This comment has been minimized.

Sign in to view
@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 2 times, most recently from 38ea006 to 18135b4 Compare October 4, 2019 21:10
@AndydeCleyre AndydeCleyre mentioned this pull request Oct 4, 2019
Closed
@AndydeCleyre AndydeCleyre mentioned this pull request Oct 8, 2019
Closed
@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 2 times, most recently from a897729 to 79db79e Compare October 18, 2019 16:46
@AndydeCleyre AndydeCleyre mentioned this pull request Nov 10, 2019
Open
atugushev
atugushev reviewed Nov 15, 2019
View reviewed changes
Copy link
Member

@atugushev atugushev left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndydeCleyre could --no-deps be implemented in a separate PR? It could be useful for this case, see #827 (comment).

@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented Nov 15, 2019 •
edited
Loading

@atugushev sure: #987

But I will note again, this more complete pull request (#907) is required to achieve consistent and predictable behavior.

Current behavior is to sometimes act like --no-deps is provided, sometimes not. So the new request #987 --pip-args provides a way to consistently achieve --no-deps behavior, but without specifying that option the user will still sometimes get that behavior and sometimes not.

@AndydeCleyre AndydeCleyre mentioned this pull request Nov 28, 2019
Closed
3 tasks
@AndydeCleyre AndydeCleyre changed the title Have diff include already-installed matching requirements in its to_install set. Have diff include already-installed matching requirements in its to_install set, so that sync consistently installs dependencies Apr 12, 2020
@AndydeCleyre
Copy link
Contributor Author

Before:

$ echo "pytest==5.1.2" > requirements.txt
$ pip-sync /dev/null
$ pip-sync requirements.txt
$ pip freeze | wc -l
12
$ pip-sync requirements.txt
$ pip freeze | wc -l
4
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4
$ pip-sync /dev/null
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4

After:

$ echo "pytest==5.1.2" > requirements.txt
$ pip-sync /dev/null
$ pip-sync requirements.txt
$ pip freeze | wc -l
12
$ pip-sync requirements.txt
$ pip freeze | wc -l
12
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4
$ pip-sync /dev/null
$ pip-sync --pip-args --no-deps requirements.txt
$ pip freeze | wc -l
4

@AndydeCleyre AndydeCleyre mentioned this pull request Aug 3, 2020
Closed
@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 2 times, most recently from 7f92b36 to 102f574 Compare January 21, 2021 04:53
@ssbarnea
Copy link
Member

@AndydeCleyre @atugushev are you still interested in getting this ready for merging? It is a very old PR.

At least the PR title and descriptions needs a little bit of simplification.

@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 4 times, most recently from 2a70c28 to 34d68de Compare April 4, 2022 20:25
@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 2 times, most recently from 0897c9b to 769ba1d Compare June 30, 2022 21:41
@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 2 times, most recently from 1b4c00a to 08a007a Compare July 18, 2022 15:47
@AndydeCleyre AndydeCleyre force-pushed the bugfix/896 branch 2 times, most recently from f90d583 to ae91e12 Compare October 3, 2022 15:21
@AndydeCleyre @webknjaz
Sync: Keep installed deps marked as to_install; Let pip sort it out
0cda3f0 
Fixes jazzband#896

By including already-installed matching requirements in the to_install set,
dependencies of pinned packages will be installed,
after potentially being uninstalled,
regardless of environment state at time of sync.

The already-installed reqs will still not be reinstalled,
thanks to pip's own handling.

The resulting state is more consistent, predictable, and practical.

Modify tests to expect explicit requirements in the to_install set, to match.
@ssbarnea
Copy link
Member

ssbarnea commented Oct 6, 2022

@atugushev I have the impression that @webknjaz took care of this one, does it look ok now? If so, lets merge it.

@ssbarnea ssbarnea added this to the 6.10.0 milestone Oct 6, 2022
@atugushev
Copy link
Member

@ssbarnea

As @AndydeCleyre said there's still no consensus on that. My point is described here. pip-sync reflects what is written in requirements.txt. If you need to install everything in requirements.txt including deps, which for some reason is absent in the file (pip-compile failed?) then pip install -r requirements.txt should be used.

BTW with the current fix, pip-sync still acts strange with "broken" requirements.txt - uninstalls and installs sub-deps on each run, see collapsed details below.

Details 
$ echo django==4.1.2 > requirements.txt

$ pip-sync
Collecting django==4.1.2
  Using cached Django-4.1.2-py3-none-any.whl (8.1 MB)
Collecting backports.zoneinfo
  Using cached backports.zoneinfo-0.2.1-cp38-cp38-macosx_10_14_x86_64.whl (35 kB)
Collecting asgiref<4,>=3.5.2
  Using cached asgiref-3.5.2-py3-none-any.whl (22 kB)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.3-py3-none-any.whl (42 kB)
Installing collected packages: sqlparse, backports.zoneinfo, asgiref, django
Successfully installed asgiref-3.5.2 backports.zoneinfo-0.2.1 django-4.1.2 sqlparse-0.4.3

$ pip-sync
Found existing installation: asgiref 3.5.2
Uninstalling asgiref-3.5.2:
  Successfully uninstalled asgiref-3.5.2
Found existing installation: backports.zoneinfo 0.2.1
Uninstalling backports.zoneinfo-0.2.1:
  Successfully uninstalled backports.zoneinfo-0.2.1
Found existing installation: sqlparse 0.4.3
Uninstalling sqlparse-0.4.3:
  Successfully uninstalled sqlparse-0.4.3
Requirement already satisfied: django==4.1.2 in ./.venv/lib/python3.8/site-packages (from -r /var/folders/l0/lnq1ghps5yqc4vkgszlcz92m0000gp/T/tmprpacgszi (line 1)) (4.1.2)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.3-py3-none-any.whl (42 kB)
Collecting asgiref<4,>=3.5.2
  Using cached asgiref-3.5.2-py3-none-any.whl (22 kB)
Collecting backports.zoneinfo
  Using cached backports.zoneinfo-0.2.1-cp38-cp38-macosx_10_14_x86_64.whl (35 kB)
Installing collected packages: sqlparse, backports.zoneinfo, asgiref
Successfully installed asgiref-3.5.2 backports.zoneinfo-0.2.1 sqlparse-0.4.3

pip-compile and pip-sync are bound together, so updating requirements.txt manually or using pip-sync as a separate tool should be discouraged in my opiion.

@webknjaz
Copy link
Member

webknjaz commented Oct 6, 2022

@ssbarnea I think I only rebased it

@webknjaz webknjaz removed their assignment Oct 6, 2022
@atugushev atugushev removed this from the 6.10.0 milestone Nov 11, 2022
@AndydeCleyre AndydeCleyre mentioned this pull request Dec 11, 2022
Closed
@atugushev
Copy link
Member

Closed based on #896 (comment).

@atugushev atugushev closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssbarnea ssbarnea ssbarnea approved these changes

@atugushev atugushev Awaiting requested review from atugushev

Assignees
No one assigned
Labels
needs discussion Need some more discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pip-sync yields different results depending on pre-installed package set
4 participants
@AndydeCleyre @ssbarnea @atugushev @webknjaz